feat(clerk-js): Link to external App page in OAuth Consent #6447
Conversation
… to support linking to external URL
e.g. http://localhost:4000/oauth-consent?scopes=email&oauth-application-name=Cursor+Chat&redirect_uri=https://google.com&logo-url=https://picsum.photos/48/48&app-url=https://google.com
🦋 Changeset detectedLatest commit: 96dc277 The changes in this PR will be included in the next version bump. This PR includes changesets to release 22 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
jfoshee
changed the title
jfoshee
changed the title
📝 WalkthroughWalkthroughThis PR adds optional external-link support for the consent UI and hardens link handling. It introduces an optional oAuthApplicationUrl parameter through types, sandbox routing, and the OAuthConsent component; adds an optional isExternal prop to ApplicationLogo so it can render external links; implements href sanitization (sanitizeHref and hasBannedHrefProtocol) and applies it in the Link primitive; expands tests for URL sanitization; and adds a VSCode task for running the sandbox. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx (1)
46-51: Confirm Avatar addsrel="noopener noreferrer"whentarget="_blank"Clickable logos point to arbitrary third-party domains. To avoid tab-napping, ensure the underlying
<a>insideAvatarsetsrel="noopener noreferrer"alongsidetarget="_blank".
If the currentAvatarimplementation does not, please patch it and involve the clerk security team.
🧹 Nitpick comments (3)
packages/clerk-js/sandbox/app.ts (1)
335-337: Guard against empty query-string values before passing to mountIf
logo-urlorapp-urlare present but empty (?logo-url=&app-url=), we still forward empty strings.__internal_mountOAuthConsentwill interpret these as “value provided”, which may cause broken<img>sources or empty<a href>attributes.- oAuthApplicationLogoUrl: searchParams.get('logo-url'), - oAuthApplicationUrl: searchParams.get('app-url'), + oAuthApplicationLogoUrl: searchParams.get('logo-url') || undefined, + oAuthApplicationUrl: searchParams.get('app-url') || undefined,packages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx (1)
20-21: Discrepancy with new prop: make URL optional in hook typings
oAuthApplicationUrlis optional but you destructure it without a default.
Ifundefined,Avatarwill still receive the prop (valueundefined) which forces a re-render in some scenarios and complicates prop-type checks.Consider eliding when falsy:
- externalLinkUrl={oAuthApplicationUrl} + externalLinkUrl={oAuthApplicationUrl ?? undefined}(or omit the prop entirely via spread).
.changeset/sad-turkeys-rhyme.md (1)
6-9: Reflect security guidance in the changesetBecause hyperlinks to third-party sites are now rendered, mention that
Avatarsanitises the URL / addsrel="noopener noreferrer". This signals consumers that the patch is safe and intentional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/sad-turkeys-rhyme.md(1 hunks).vscode/tasks.json(1 hunks)packages/clerk-js/sandbox/app.ts(1 hunks)packages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx(3 hunks)packages/clerk-js/src/ui/elements/Avatar.tsx(3 hunks)packages/types/src/clerk.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
packages/clerk-js/sandbox/app.tspackages/clerk-js/src/ui/elements/Avatar.tsxpackages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsxpackages/types/src/clerk.ts
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/sandbox/app.tspackages/clerk-js/src/ui/elements/Avatar.tsxpackages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsxpackages/types/src/clerk.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/sandbox/app.tspackages/clerk-js/src/ui/elements/Avatar.tsxpackages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsxpackages/types/src/clerk.ts
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/clerk-js/sandbox/app.tspackages/clerk-js/src/ui/elements/Avatar.tsxpackages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsxpackages/types/src/clerk.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
packages/clerk-js/sandbox/app.tspackages/clerk-js/src/ui/elements/Avatar.tsxpackages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsxpackages/types/src/clerk.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
packages/clerk-js/sandbox/app.tspackages/clerk-js/src/ui/elements/Avatar.tsxpackages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsxpackages/types/src/clerk.ts
**/*
⚙️ CodeRabbit Configuration File
**/*: If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.Whenever reviewing a pull request, if there are any changes that could impact security, always tag
@clerk/securityin the PR.Security-impacting changes include, but are not limited to:
- Changes to authentication logic or mechanisms (e.g. login, session handling, token issuance)
- Any modification to access control, authorization checks, or role-based permissions
- Introduction or modification of hashing algorithms, signature verification, or cryptographic primitives
- Handling of sensitive data (e.g. passwords, tokens, secrets, PII)
- Integration with external identity providers (e.g. SSO, OAuth, OpenID Connect)
- Modifications to security headers, cookie flags, CORS policies, or CSRF protections
- Bypass mechanisms (e.g. feature flags, testing overrides) that could weaken protections
- Changes to rate limiting, abuse prevention, or input validation
If you're unsure whether a change is security-relevant, err on the side of caution and tag
@clerk/security.Any time that you tag
@clerk/security, please do so explicitly in a code comment, rather than within a collapsed section in a coderabbit comment, such as the "recent review details" section. If you do use the team name in any thinking or non-direct-code-comment content, it can be referred to as "clerk security team" to avoid accidentally printing the tag which sends a notification to the team.
Files:
packages/clerk-js/sandbox/app.tspackages/clerk-js/src/ui/elements/Avatar.tsxpackages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsxpackages/types/src/clerk.ts
packages/clerk-js/src/ui/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/clerk-js-ui.mdc)
packages/clerk-js/src/ui/**/*.{ts,tsx}: Element descriptors should always be camelCase
Use element descriptors in UI components to enable consistent theming and styling via appearance.elements
Element descriptors should generate unique, stable CSS classes for theming
Element descriptors should handle state classes (e.g., cl-loading, cl-active, cl-error, cl-open) automatically based on component state
Do not render hard-coded values; all user-facing strings must be localized using provided localization methods
Use the useLocalizations hook and localizationKeys utility for all text and error messages
Use the styled system (sx prop, theme tokens, responsive values) for custom component styling
Use useCardState for card-level state, useFormState for form-level state, and useLoadingStatus for loading states
Always use handleError utility for API errors and use translateError for localized error messages
Use useFormControl for form field state, implement proper validation, and handle loading and error states in forms
Use localization keys for all form labels and placeholders
Use element descriptors for consistent styling and follow the theme token system
Use the Card and FormContainer patterns for consistent UI structure
Files:
packages/clerk-js/src/ui/elements/Avatar.tsxpackages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx
**/*.{jsx,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
**/*.{jsx,tsx}: Use error boundaries in React components
Minimize re-renders in React components
**/*.{jsx,tsx}: Always use functional components with hooks instead of class components
Follow PascalCase naming for components:UserProfile,NavigationMenu
Keep components focused on a single responsibility - split large components
Limit component size to 150-200 lines; extract logic into custom hooks
Use composition over inheritance - prefer smaller, composable components
Export components as named exports for better tree-shaking
One component per file with matching filename and component name
Use useState for simple state management
Use useReducer for complex state logic
Implement proper state initialization
Use proper state updates with callbacks
Implement proper state cleanup
Use Context API for theme/authentication
Implement proper state selectors
Use proper state normalization
Implement proper state persistence
Use React.memo for expensive components
Implement proper useCallback for handlers
Use proper useMemo for expensive computations
Implement proper virtualization for lists
Use proper code splitting with React.lazy
Implement proper cleanup in useEffect
Use proper refs for DOM access
Implement proper event listener cleanup
Use proper abort controllers for fetch
Implement proper subscription cleanup
Use proper HTML elements
Implement proper ARIA attributes
Use proper heading hierarchy
Implement proper form labels
Use proper button types
Implement proper focus management
Use proper keyboard shortcuts
Implement proper tab order
Use proper skip links
Implement proper focus traps
Implement proper error boundaries
Use proper error logging
Implement proper error recovery
Use proper error messages
Implement proper error fallbacks
Use proper form validation
Implement proper error states
Use proper error messages
Implement proper form submission
Use proper form reset
Use proper component naming
Implement proper file naming
Use proper prop naming
Implement proper...
Files:
packages/clerk-js/src/ui/elements/Avatar.tsxpackages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx
**/*.tsx
📄 CodeRabbit Inference Engine (.cursor/rules/react.mdc)
**/*.tsx: Use proper type definitions for props and state
Leverage TypeScript's type inference where possible
Use proper event types for handlers
Implement proper generic types for reusable components
Use proper type guards for conditional rendering
Files:
packages/clerk-js/src/ui/elements/Avatar.tsxpackages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx
.changeset/**
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Automated releases must use Changesets.
Files:
.changeset/sad-turkeys-rhyme.md
🔇 Additional comments (4)
.vscode/tasks.json (1)
1-13: VSCode task looks fineTask follows VSCode 2.0 schema and scopes execution correctly to
packages/clerk-js.
No action required.packages/clerk-js/src/ui/elements/Avatar.tsx (3)
18-22: LGTM! Clear and comprehensive JSDoc documentation.The JSDoc comment clearly describes the prop's purpose and behavior, including the important detail about opening in a new tab.
36-36: LGTM! Proper prop destructuring.The destructuring follows the established pattern and is properly typed.
74-84: Good security practices, but consider additional validation and accessibility improvements.The implementation correctly uses
rel="noopener noreferrer"for security, which prevents the new page from accessingwindow.openerand blocks referrer information. The trim check is also a good practice.However, consider these improvements:
Security considerations:
- Add URL validation to ensure only valid, safe URLs are accepted
- Consider sanitizing the URL before using it in
hrefto prevent XSS- Since this handles external URLs (OAuth app URLs), validate that URLs use safe protocols (https://)
Accessibility improvements:
- Add
aria-labelortitleto indicate this is an external link- Consider adding visual indication for external links
{externalLinkUrl && externalLinkUrl.trim() ? ( <a href={externalLinkUrl} target='_blank' rel='noopener noreferrer' + aria-label={`${title}'s external page (opens in new tab)`} > {ImgOrFallback} </a> ) : ( ImgOrFallback )}@clerk/security - This change introduces external URL handling in the Avatar component, which will be used for OAuth application URLs. Please review the security implications of accepting external URLs without validation.
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/elements
@clerk/clerk-expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/clerk-react
@clerk/react-router
@clerk/remix
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/themes
@clerk/types
@clerk/upgrade
@clerk/vue
commit: |
add noreferrer to Link because it is undesirable to share all the consent query params
and just apply isExternal if the link is absolute so this is more flexible
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/clerk-js/src/ui/primitives/Link.tsx (1)
61-67: Add protocol sanitization to Link’s href for external linksThe
Linkprimitive currently rendershrefunfiltered, so maliciousjavascript:,data:or other unsafe schemes could slip through whenisExternalis true. We should leverage the existing utilities inpackages/clerk-js/src/utils/url.tsto block non-http(s) protocols:• Import and use
hasBannedProtocolandisDataUri(orisProblematicUrl) fromutils/url.ts.
• Compute a “safe” href before rendering and fall back toundefinedor'#'if the URL is unsafe.
• Only applytarget="_blank"/rel="noopener noreferrer"when the sanitized URL passes the checks.Example diff inside
packages/clerk-js/src/ui/primitives/Link.tsx:import React from 'react'; +import { hasBannedProtocol, isDataUri } from '../../utils/url'; export const Link = (props: LinkProps): JSX.Element => { const { isExternal, children, href, onClick, ...rest } = props; + // Block unsafe external URL schemes + const safeHref = + href && isExternal && (hasBannedProtocol(href) || isDataUri(href)) + ? undefined + : href; const onClickHandler = onClick ? (e: React.MouseEvent<HTMLAnchorElement, MouseEvent>) => {And update the JSX:
- <a - href={href || ''} + <a + href={safeHref || ''} target={href && isExternal ? '_blank' : undefined} rel={href && isExternal ? 'noopener noreferrer' : undefined}This ensures that any
javascript:ordata:URLs are never set on external links.
🧹 Nitpick comments (1)
packages/clerk-js/src/ui/primitives/Link.tsx (1)
61-67: Optional hardening: add referrerPolicy for explicit no‑referrer behaviorWhile rel="noreferrer" is widely supported, explicitly setting referrerPolicy improves clarity and coverage.
Apply this minimal addition:
target={href && isExternal ? '_blank' : undefined} - rel={href && isExternal ? 'noopener noreferrer' : undefined} + rel={href && isExternal ? 'noopener noreferrer' : undefined} + referrerPolicy={href && isExternal ? 'no-referrer' : undefined}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx(3 hunks)packages/clerk-js/src/ui/elements/Avatar.tsx(4 hunks)packages/clerk-js/src/ui/primitives/Link.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx
- packages/clerk-js/src/ui/elements/Avatar.tsx
🧰 Additional context used
📓 Path-based instructions (10)
packages/clerk-js/src/ui/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/clerk-js-ui.mdc)
packages/clerk-js/src/ui/**/*.{ts,tsx}: Element descriptors should always be camelCase
Use element descriptors in UI components to enable consistent theming and styling via appearance.elements
Element descriptors should generate unique, stable CSS classes for theming
Element descriptors should handle state classes (e.g., cl-loading, cl-active, cl-error, cl-open) automatically based on component state
Do not render hard-coded values; all user-facing strings must be localized using provided localization methods
Use the useLocalizations hook and localizationKeys utility for all text and error messages
Use the styled system (sx prop, theme tokens, responsive values) for custom component styling
Use useCardState for card-level state, useFormState for form-level state, and useLoadingStatus for loading states
Always use handleError utility for API errors and use translateError for localized error messages
Use useFormControl for form field state, implement proper validation, and handle loading and error states in forms
Use localization keys for all form labels and placeholders
Use element descriptors for consistent styling and follow the theme token system
Use the Card and FormContainer patterns for consistent UI structure
Files:
packages/clerk-js/src/ui/primitives/Link.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
packages/clerk-js/src/ui/primitives/Link.tsx
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/ui/primitives/Link.tsx
packages/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/ui/primitives/Link.tsx
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/clerk-js/src/ui/primitives/Link.tsx
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
packages/clerk-js/src/ui/primitives/Link.tsx
**/*.{jsx,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
**/*.{jsx,tsx}: Use error boundaries in React components
Minimize re-renders in React components
**/*.{jsx,tsx}: Always use functional components with hooks instead of class components
Follow PascalCase naming for components:UserProfile,NavigationMenu
Keep components focused on a single responsibility - split large components
Limit component size to 150-200 lines; extract logic into custom hooks
Use composition over inheritance - prefer smaller, composable components
Export components as named exports for better tree-shaking
One component per file with matching filename and component name
Use useState for simple state management
Use useReducer for complex state logic
Implement proper state initialization
Use proper state updates with callbacks
Implement proper state cleanup
Use Context API for theme/authentication
Implement proper state selectors
Use proper state normalization
Implement proper state persistence
Use React.memo for expensive components
Implement proper useCallback for handlers
Use proper useMemo for expensive computations
Implement proper virtualization for lists
Use proper code splitting with React.lazy
Implement proper cleanup in useEffect
Use proper refs for DOM access
Implement proper event listener cleanup
Use proper abort controllers for fetch
Implement proper subscription cleanup
Use proper HTML elements
Implement proper ARIA attributes
Use proper heading hierarchy
Implement proper form labels
Use proper button types
Implement proper focus management
Use proper keyboard shortcuts
Implement proper tab order
Use proper skip links
Implement proper focus traps
Implement proper error boundaries
Use proper error logging
Implement proper error recovery
Use proper error messages
Implement proper error fallbacks
Use proper form validation
Implement proper error states
Use proper error messages
Implement proper form submission
Use proper form reset
Use proper component naming
Implement proper file naming
Use proper prop naming
Implement proper...
Files:
packages/clerk-js/src/ui/primitives/Link.tsx
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
packages/clerk-js/src/ui/primitives/Link.tsx
**/*.tsx
📄 CodeRabbit Inference Engine (.cursor/rules/react.mdc)
**/*.tsx: Use proper type definitions for props and state
Leverage TypeScript's type inference where possible
Use proper event types for handlers
Implement proper generic types for reusable components
Use proper type guards for conditional rendering
Files:
packages/clerk-js/src/ui/primitives/Link.tsx
**/*
⚙️ CodeRabbit Configuration File
If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.
Files:
packages/clerk-js/src/ui/primitives/Link.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
packages/clerk-js/src/ui/primitives/Link.tsx (1)
66-66: Add rel="noopener noreferrer" for external links — correct and securePrevents reverse‑tabnabbing and suppresses Referer. Matches best practices for target=_blank external links.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx(3 hunks)packages/clerk-js/src/ui/primitives/Link.tsx(1 hunks)packages/types/src/clerk.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/types/src/clerk.ts
- packages/clerk-js/src/ui/primitives/Link.tsx
🧰 Additional context used
📓 Path-based instructions (10)
packages/clerk-js/src/ui/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/clerk-js-ui.mdc)
packages/clerk-js/src/ui/**/*.{ts,tsx}: Element descriptors should always be camelCase
Use element descriptors in UI components to enable consistent theming and styling via appearance.elements
Element descriptors should generate unique, stable CSS classes for theming
Element descriptors should handle state classes (e.g., cl-loading, cl-active, cl-error, cl-open) automatically based on component state
Do not render hard-coded values; all user-facing strings must be localized using provided localization methods
Use the useLocalizations hook and localizationKeys utility for all text and error messages
Use the styled system (sx prop, theme tokens, responsive values) for custom component styling
Use useCardState for card-level state, useFormState for form-level state, and useLoadingStatus for loading states
Always use handleError utility for API errors and use translateError for localized error messages
Use useFormControl for form field state, implement proper validation, and handle loading and error states in forms
Use localization keys for all form labels and placeholders
Use element descriptors for consistent styling and follow the theme token system
Use the Card and FormContainer patterns for consistent UI structure
Files:
packages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
packages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx
packages/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
packages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx
**/*.{jsx,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
**/*.{jsx,tsx}: Use error boundaries in React components
Minimize re-renders in React components
**/*.{jsx,tsx}: Always use functional components with hooks instead of class components
Follow PascalCase naming for components:UserProfile,NavigationMenu
Keep components focused on a single responsibility - split large components
Limit component size to 150-200 lines; extract logic into custom hooks
Use composition over inheritance - prefer smaller, composable components
Export components as named exports for better tree-shaking
One component per file with matching filename and component name
Use useState for simple state management
Use useReducer for complex state logic
Implement proper state initialization
Use proper state updates with callbacks
Implement proper state cleanup
Use Context API for theme/authentication
Implement proper state selectors
Use proper state normalization
Implement proper state persistence
Use React.memo for expensive components
Implement proper useCallback for handlers
Use proper useMemo for expensive computations
Implement proper virtualization for lists
Use proper code splitting with React.lazy
Implement proper cleanup in useEffect
Use proper refs for DOM access
Implement proper event listener cleanup
Use proper abort controllers for fetch
Implement proper subscription cleanup
Use proper HTML elements
Implement proper ARIA attributes
Use proper heading hierarchy
Implement proper form labels
Use proper button types
Implement proper focus management
Use proper keyboard shortcuts
Implement proper tab order
Use proper skip links
Implement proper focus traps
Implement proper error boundaries
Use proper error logging
Implement proper error recovery
Use proper error messages
Implement proper error fallbacks
Use proper form validation
Implement proper error states
Use proper error messages
Implement proper form submission
Use proper form reset
Use proper component naming
Implement proper file naming
Use proper prop naming
Implement proper...
Files:
packages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
packages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx
**/*.tsx
📄 CodeRabbit Inference Engine (.cursor/rules/react.mdc)
**/*.tsx: Use proper type definitions for props and state
Leverage TypeScript's type inference where possible
Use proper event types for handlers
Implement proper generic types for reusable components
Use proper type guards for conditional rendering
Files:
packages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx
**/*
⚙️ CodeRabbit Configuration File
If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.
Files:
packages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
packages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx (1)
49-51: No change needed for ApplicationLogo’shrefpropThe
ApplicationLogocomponent is designed to accept anhrefprop and wrap the logo image in your internalRouterLinkimplementation—not theAvatarcomponent—so there is nolinkUrlAPI to forward. You can safely ignore the earlier suggestion to rename or map this prop.Likely an incorrect or invalid review comment.
…fely open the link in a new window
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (4)
packages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx (3)
102-105: Add JSDoc for the new localization stringsThe component uses hardcoded strings in the Header.Subtitle that should be properly localized. According to the coding guidelines, all user-facing strings must use localization methods.
Update the localization approach:
<Header.Subtitle - localizationKey={`wants to access ${applicationName} on behalf of ${primaryEmailAddress}`} + localizationKey='oauthConsent.subtitle' + localizationOptions={{ applicationName, userEmail: primaryEmailAddress }} />
128-129: Replace template literals with proper localization keysThese strings use template literals instead of proper localization keys, violating the guideline that all user-facing strings must be localized using provided localization methods.
Replace with proper localization keys:
<Text variant='subtitle' - localizationKey={`This will allow ${oAuthApplicationName} access to:`} + localizationKey='oauthConsent.scopesIntro' + localizationOptions={{ appName: oAuthApplicationName }} /> <Header.Subtitle - localizationKey={`Make sure you trust ${oAuthApplicationName} and that this URL belongs to ${oAuthApplicationName}.`} + localizationKey='oauthConsent.redirectUrlWarning' + localizationOptions={{ appName: oAuthApplicationName }} />Also applies to: 287-289
19-262: Add comprehensive tests for OAuthConsentInternalThere are currently no existing specs covering the OAuth consent component. Please add unit tests to cover:
- Rendering consent flow with valid
http/httpsredirect URLs- Handling invalid or malicious
redirectUrl(no external link rendered)- Behavior when no URL is provided (static logo only)
- External application links opening with
target="_blank"andrel="noopener noreferrer"- All avatar scenarios:
- Both OAuth app & Clerk app logos present
- Only OAuth app logo
- Only Clerk app logo
- Neither logo present
packages/clerk-js/src/ui/elements/ApplicationLogo.tsx (1)
25-44: Add tests for ApplicationLogo’s external/internal link logicNo existing tests reference
ApplicationLogounderpackages/clerk-js/src. Please create a new test file alongside the component, for example:
- packages/clerk-js/src/ui/elements/ApplicationLogo.test.tsx
Include the following scenarios:
isExternal=truerenders<Link>withtarget="_blank"andrel="noopener noreferrer"isExternal=false(or undefined) renders<RouterLink>- Both cases properly pass through
href,src, andaltprops
♻️ Duplicate comments (1)
packages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx (1)
20-21: Add URL sanitization to prevent malicious schemesThe
oAuthApplicationUrlis directly passed toApplicationLogowithout validation. This could allow malicious schemes likejavascript:ordata:URLs to be executed when users click the logo.As noted in the previous review, add URL protocol validation:
export function OAuthConsentInternal() { const { scopes, oAuthApplicationName, oAuthApplicationLogoUrl, oAuthApplicationUrl, redirectUrl, onDeny, onAllow } = useOAuthConsentContext(); + const isValidOAuthUrl = + oAuthApplicationUrl?.startsWith('http://') || + oAuthApplicationUrl?.startsWith('https://');Then update the
hrefprop usage in lines 49-50 and 70-71:- href={oAuthApplicationUrl} + href={isValidOAuthUrl ? oAuthApplicationUrl : undefined}
🧹 Nitpick comments (2)
packages/clerk-js/src/ui/elements/ApplicationLogo.tsx (2)
38-43: Improve JSDoc documentation for the new isExternal propThe JSDoc is clear but could be more specific about the security implications and relationship to the
hrefprop.Enhance the documentation:
/** + * Whether the href should be treated as an external link. - * When true, uses a Link component with target="_blank" and proper security attributes. + * When true, uses a Link component with target="_blank" and rel="noopener noreferrer" for security. - * When false or undefined, uses RouterLink for internal navigation. + * When false or undefined, uses RouterLink for internal navigation within the application. + * Only takes effect when href is provided. */ isExternal?: boolean;
90-105: Consider extracting logo link logic into a helper componentThe conditional rendering logic for external vs internal links could be extracted into a reusable component to improve readability and maintainability.
Consider creating a helper component:
+const LogoLink: React.FC<{ + logoUrl: string; + isExternal?: boolean; + children: React.ReactNode +}> = ({ logoUrl, isExternal, children }) => { + return isExternal ? ( + <Link focusRing href={logoUrl} isExternal> + {children} + </Link> + ) : ( + <RouterLink focusRing to={logoUrl}> + {children} + </RouterLink> + ); +}; return ( <Flex elementDescriptor={descriptors.logoBox} {...rest} sx={[...]} > {logoUrl ? ( - isExternal ? ( - <Link focusRing href={logoUrl} isExternal> - {image} - </Link> - ) : ( - <RouterLink focusRing to={logoUrl}> - {image} - </RouterLink> - ) + <LogoLink logoUrl={logoUrl} isExternal={isExternal}> + {image} + </LogoLink> ) : ( image )} </Flex> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx(3 hunks)packages/clerk-js/src/ui/elements/ApplicationLogo.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
packages/clerk-js/src/ui/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/clerk-js-ui.mdc)
packages/clerk-js/src/ui/**/*.{ts,tsx}: Element descriptors should always be camelCase
Use element descriptors in UI components to enable consistent theming and styling via appearance.elements
Element descriptors should generate unique, stable CSS classes for theming
Element descriptors should handle state classes (e.g., cl-loading, cl-active, cl-error, cl-open) automatically based on component state
Do not render hard-coded values; all user-facing strings must be localized using provided localization methods
Use the useLocalizations hook and localizationKeys utility for all text and error messages
Use the styled system (sx prop, theme tokens, responsive values) for custom component styling
Use useCardState for card-level state, useFormState for form-level state, and useLoadingStatus for loading states
Always use handleError utility for API errors and use translateError for localized error messages
Use useFormControl for form field state, implement proper validation, and handle loading and error states in forms
Use localization keys for all form labels and placeholders
Use element descriptors for consistent styling and follow the theme token system
Use the Card and FormContainer patterns for consistent UI structure
Files:
packages/clerk-js/src/ui/elements/ApplicationLogo.tsxpackages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
packages/clerk-js/src/ui/elements/ApplicationLogo.tsxpackages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/ui/elements/ApplicationLogo.tsxpackages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx
packages/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/ui/elements/ApplicationLogo.tsxpackages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/clerk-js/src/ui/elements/ApplicationLogo.tsxpackages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
packages/clerk-js/src/ui/elements/ApplicationLogo.tsxpackages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx
**/*.{jsx,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
**/*.{jsx,tsx}: Use error boundaries in React components
Minimize re-renders in React components
**/*.{jsx,tsx}: Always use functional components with hooks instead of class components
Follow PascalCase naming for components:UserProfile,NavigationMenu
Keep components focused on a single responsibility - split large components
Limit component size to 150-200 lines; extract logic into custom hooks
Use composition over inheritance - prefer smaller, composable components
Export components as named exports for better tree-shaking
One component per file with matching filename and component name
Use useState for simple state management
Use useReducer for complex state logic
Implement proper state initialization
Use proper state updates with callbacks
Implement proper state cleanup
Use Context API for theme/authentication
Implement proper state selectors
Use proper state normalization
Implement proper state persistence
Use React.memo for expensive components
Implement proper useCallback for handlers
Use proper useMemo for expensive computations
Implement proper virtualization for lists
Use proper code splitting with React.lazy
Implement proper cleanup in useEffect
Use proper refs for DOM access
Implement proper event listener cleanup
Use proper abort controllers for fetch
Implement proper subscription cleanup
Use proper HTML elements
Implement proper ARIA attributes
Use proper heading hierarchy
Implement proper form labels
Use proper button types
Implement proper focus management
Use proper keyboard shortcuts
Implement proper tab order
Use proper skip links
Implement proper focus traps
Implement proper error boundaries
Use proper error logging
Implement proper error recovery
Use proper error messages
Implement proper error fallbacks
Use proper form validation
Implement proper error states
Use proper error messages
Implement proper form submission
Use proper form reset
Use proper component naming
Implement proper file naming
Use proper prop naming
Implement proper...
Files:
packages/clerk-js/src/ui/elements/ApplicationLogo.tsxpackages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
packages/clerk-js/src/ui/elements/ApplicationLogo.tsxpackages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx
**/*.tsx
📄 CodeRabbit Inference Engine (.cursor/rules/react.mdc)
**/*.tsx: Use proper type definitions for props and state
Leverage TypeScript's type inference where possible
Use proper event types for handlers
Implement proper generic types for reusable components
Use proper type guards for conditional rendering
Files:
packages/clerk-js/src/ui/elements/ApplicationLogo.tsxpackages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx
**/*
⚙️ CodeRabbit Configuration File
If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.
Files:
packages/clerk-js/src/ui/elements/ApplicationLogo.tsxpackages/clerk-js/src/ui/components/OAuthConsent/OAuthConsent.tsx
🧬 Code Graph Analysis (1)
packages/clerk-js/src/ui/elements/ApplicationLogo.tsx (1)
packages/clerk-js/src/ui/primitives/Link.tsx (1)
Link(57-81)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
packages/clerk-js/src/ui/elements/ApplicationLogo.tsx (1)
47-47: LGTM! Well-implemented external link supportThe implementation correctly:
- Adds the
isExternalprop to control link behavior- Uses conditional rendering to choose between
Link(external) andRouterLink(internal)- Passes
focusRingto both link types for accessibility- Maintains backward compatibility when
isExternalis not providedThe external links will benefit from the security attributes (
rel="noopener noreferrer") implemented in theLinkprimitive.Also applies to: 90-105
… Link component There is an existing function `hasBannedProtocol` which explicitly allows data: protocol in a test. So I did not use that function as data: is dangerous for links.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/clerk-js/src/utils/__tests__/url.spec.ts (1)
152-170: Strengthen hasBannedHrefProtocol test cases with mixed-case and whitespace schemesThe sanitizeHref suite already covers uppercase/whitespace variants. Add parity tests here to assert protocol normalization through URL() is consistently handled.
Apply this diff to extend the cases:
const cases: Array<[string, boolean]> = [ ['https://www.clerk.com/', false], ['http://www.clerk.com/', false], ['/sign-in', false], ['/sign-in?test=1', false], ['/?test', false], ['javascript:console.log(document.cookies)', true], + ['JAVASCRIPT:alert(1)', true], + [' JavaScript:alert(1) ', true], ['data:image/png;base64,iVBORw0KGgoAAA5ErkJggg==', true], ['vbscript:alert("xss")', true], ['blob:https://example.com/12345678-1234-1234-1234-123456789012', true], ['ftp://files.example.com/file.txt', false], ['mailto:[email protected]', false], ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/clerk-js/src/ui/primitives/Link.tsx(3 hunks)packages/clerk-js/src/utils/__tests__/url.spec.ts(4 hunks)packages/clerk-js/src/utils/url.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (13)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
packages/clerk-js/src/utils/__tests__/url.spec.tspackages/clerk-js/src/utils/url.tspackages/clerk-js/src/ui/primitives/Link.tsx
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/utils/__tests__/url.spec.tspackages/clerk-js/src/utils/url.tspackages/clerk-js/src/ui/primitives/Link.tsx
packages/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/utils/__tests__/url.spec.tspackages/clerk-js/src/utils/url.tspackages/clerk-js/src/ui/primitives/Link.tsx
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/clerk-js/src/utils/__tests__/url.spec.tspackages/clerk-js/src/utils/url.tspackages/clerk-js/src/ui/primitives/Link.tsx
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
packages/clerk-js/src/utils/__tests__/url.spec.tspackages/clerk-js/src/utils/url.tspackages/clerk-js/src/ui/primitives/Link.tsx
packages/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Unit tests should use Jest or Vitest as the test runner.
Files:
packages/clerk-js/src/utils/__tests__/url.spec.ts
packages/{clerk-js,elements,themes}/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Visual regression testing should be performed for UI components.
Files:
packages/clerk-js/src/utils/__tests__/url.spec.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
packages/clerk-js/src/utils/__tests__/url.spec.tspackages/clerk-js/src/utils/url.tspackages/clerk-js/src/ui/primitives/Link.tsx
**/__tests__/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/typescript.mdc)
**/__tests__/**/*.{ts,tsx}: Create type-safe test builders/factories
Use branded types for test isolation
Implement proper mock types that match interfaces
Files:
packages/clerk-js/src/utils/__tests__/url.spec.ts
**/*
⚙️ CodeRabbit Configuration File
If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.
Files:
packages/clerk-js/src/utils/__tests__/url.spec.tspackages/clerk-js/src/utils/url.tspackages/clerk-js/src/ui/primitives/Link.tsx
packages/clerk-js/src/ui/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/clerk-js-ui.mdc)
packages/clerk-js/src/ui/**/*.{ts,tsx}: Element descriptors should always be camelCase
Use element descriptors in UI components to enable consistent theming and styling via appearance.elements
Element descriptors should generate unique, stable CSS classes for theming
Element descriptors should handle state classes (e.g., cl-loading, cl-active, cl-error, cl-open) automatically based on component state
Do not render hard-coded values; all user-facing strings must be localized using provided localization methods
Use the useLocalizations hook and localizationKeys utility for all text and error messages
Use the styled system (sx prop, theme tokens, responsive values) for custom component styling
Use useCardState for card-level state, useFormState for form-level state, and useLoadingStatus for loading states
Always use handleError utility for API errors and use translateError for localized error messages
Use useFormControl for form field state, implement proper validation, and handle loading and error states in forms
Use localization keys for all form labels and placeholders
Use element descriptors for consistent styling and follow the theme token system
Use the Card and FormContainer patterns for consistent UI structure
Files:
packages/clerk-js/src/ui/primitives/Link.tsx
**/*.{jsx,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
**/*.{jsx,tsx}: Use error boundaries in React components
Minimize re-renders in React components
**/*.{jsx,tsx}: Always use functional components with hooks instead of class components
Follow PascalCase naming for components:UserProfile,NavigationMenu
Keep components focused on a single responsibility - split large components
Limit component size to 150-200 lines; extract logic into custom hooks
Use composition over inheritance - prefer smaller, composable components
Export components as named exports for better tree-shaking
One component per file with matching filename and component name
Use useState for simple state management
Use useReducer for complex state logic
Implement proper state initialization
Use proper state updates with callbacks
Implement proper state cleanup
Use Context API for theme/authentication
Implement proper state selectors
Use proper state normalization
Implement proper state persistence
Use React.memo for expensive components
Implement proper useCallback for handlers
Use proper useMemo for expensive computations
Implement proper virtualization for lists
Use proper code splitting with React.lazy
Implement proper cleanup in useEffect
Use proper refs for DOM access
Implement proper event listener cleanup
Use proper abort controllers for fetch
Implement proper subscription cleanup
Use proper HTML elements
Implement proper ARIA attributes
Use proper heading hierarchy
Implement proper form labels
Use proper button types
Implement proper focus management
Use proper keyboard shortcuts
Implement proper tab order
Use proper skip links
Implement proper focus traps
Implement proper error boundaries
Use proper error logging
Implement proper error recovery
Use proper error messages
Implement proper error fallbacks
Use proper form validation
Implement proper error states
Use proper error messages
Implement proper form submission
Use proper form reset
Use proper component naming
Implement proper file naming
Use proper prop naming
Implement proper...
Files:
packages/clerk-js/src/ui/primitives/Link.tsx
**/*.tsx
📄 CodeRabbit Inference Engine (.cursor/rules/react.mdc)
**/*.tsx: Use proper type definitions for props and state
Leverage TypeScript's type inference where possible
Use proper event types for handlers
Implement proper generic types for reusable components
Use proper type guards for conditional rendering
Files:
packages/clerk-js/src/ui/primitives/Link.tsx
🧬 Code Graph Analysis (2)
packages/clerk-js/src/utils/__tests__/url.spec.ts (1)
packages/clerk-js/src/utils/url.ts (2)
hasBannedHrefProtocol(307-313)sanitizeHref(323-350)
packages/clerk-js/src/ui/primitives/Link.tsx (1)
packages/clerk-js/src/utils/url.ts (1)
sanitizeHref(323-350)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (6)
packages/clerk-js/src/utils/url.ts (2)
282-299: Docstring clarity: OKClear distinction between generic banned protocol check vs href-specific checks is helpful. No issues.
24-26: It looks like banningfile:/filesystem:in URL sanitization only impacts anchor‐href checks. We didn’t find any in-app link flows relying on either scheme:
- All
file:occurrences are in package.json (local installs), origin tests (file://), or build configs (metro.config), not rendered<a href>.createObjectURLusages yieldblob:URLs, which are already banned.- URL sanitization tests cover
blob:but never whitelistfile:orfilesystem:.Update banned protocols as suggested
--- a/packages/clerk-js/src/utils/url.ts +++ b/packages/clerk-js/src/utils/url.ts @@ -23,2 +23,4 @@ -const BANNED_HREF_PROTOCOLS = ['javascript:', 'data:', 'vbscript:', 'blob:'] as const; +// Protocols dangerous for `<a href>` to prevent XSS or local file access. +// Also disallow `file:`/`filesystem:` to block navigation to local files. +const BANNED_HREF_PROTOCOLS = ['javascript:', 'data:', 'vbscript:', 'blob:', 'file:', 'filesystem:'] as const;No other code paths rely on
file:/filesystem:in user‐facing links—tests and configs remain unaffected.packages/clerk-js/src/ui/primitives/Link.tsx (2)
61-63: LGTM: sanitize href before useGood call to sanitize ahead of time and reuse the result for both navigation guard and attributes.
64-71: Guarding navigation via sanitizedHref is correctPreventing default when sanitizedHref is null blocks dangerous protocols early. This aligns with the new utility.
packages/clerk-js/src/utils/__tests__/url.spec.ts (2)
10-10: LGTM: add tests for new exportsImporting hasBannedHrefProtocol and sanitizeHref for dedicated coverage is appropriate.
Also applies to: 22-22
172-222: Ensure tests and documentation match the intended policy for file://, filesystem:, and protocol-relative URLsPlease update your test suite (and JSDoc) to reflect the actual behavior or intended policy:
• Add a protocol-relative case:
['//cdn.example.com/asset.css', '//cdn.example.com/asset.css']• Decide on
file:andfilesystem:schemes:
– If you want to ban them, extend BANNED_HREF_PROTOCOLS with'file:'and'filesystem:', then add tests expectingnullfor:
ts ['file:///etc/passwd', null], ['filesystem:https://example.com/temporary/file.txt', null],
– If you intend to allow them, add tests expecting the input unchanged:
ts ['file:///etc/passwd', 'file:///etc/passwd'], ['filesystem:https://example.com/temporary/file.txt', 'filesystem:https://example.com/temporary/file.txt'],
and update the sanitizeHref JSDoc to state that onlyjavascript:,data:,vbscript:, andblob:are considered dangerous.
| href={sanitizedHref || ''} | ||
| target={sanitizedHref && isExternal ? '_blank' : undefined} | ||
| rel={sanitizedHref && isExternal ? 'noopener noreferrer' : undefined} |
There was a problem hiding this comment.
Avoid href="" to prevent unintended navigation; preserve user-supplied rel/target when not external
- href="" causes a same-page navigation (jump-to-top) if onClick is not provided. Prefer omitting href entirely when sanitizedHref is null.
- Minor: current props override rel/target even when not external, which can drop user-provided values spread via rest. Consider conditional props to avoid overriding unless external.
Apply this minimal diff for href to prevent spurious navigation:
- href={sanitizedHref || ''}
+ href={sanitizedHref ?? undefined}Optionally (outside the selected lines), rework attribute merging to preserve user-specified rel/target when not external:
// Replace the <a ...> opening tag with:
<a
{...applyDataStateProps(filterProps(rest))}
onClick={onClickHandler}
href={sanitizedHref ?? undefined}
{...(sanitizedHref && isExternal ? { target: '_blank', rel: 'noopener noreferrer' } : {})}
css={applyVariants(props) as any}
>You may also consider setting aria-disabled and tabIndex when sanitizedHref is null to improve a11y:
- aria-disabled={!sanitizedHref}
- tabIndex={!sanitizedHref ? -1 : undefined}
🤖 Prompt for AI Agents
In packages/clerk-js/src/ui/primitives/Link.tsx around lines 77-79, the
component currently sets href to an empty string and unconditionally overrides
target/rel which causes same-page navigation and drops user-provided rel/target;
change href to use undefined when sanitizedHref is null/undefined
(href={sanitizedHref ?? undefined}) and only set target/rel when sanitizedHref
&& isExternal (e.g. spread a conditional object for target/rel) so you don’t
override rest props; optionally add aria-disabled={!sanitizedHref} and
tabIndex={!sanitizedHref ? -1 : undefined} for better a11y.
There was a problem hiding this comment.
I agree we should set it to undefined, but it is not backwards compatible. Seems some consumers of Link expect href to be there even when it is not set. Perhaps for styling or mouse cursor.
packages/clerk-js/src/ui/elements/tests/PlainInput.spec.tsx#124
expect(getByText(/take action/i, { selector: 'a' })).toHaveAttribute('href', '');
There was a problem hiding this comment.
🧩 Analysis chain
@jfoshee You raise an excellent point about backward compatibility! Thank you for catching this and providing the specific test case.
You're absolutely right that changing href="" to href={undefined} could break existing consumers that depend on the href attribute being present, even when empty. This includes:
- CSS selectors that target
a[href] - Tests that explicitly check for the presence of
href="" - Browser behavior differences between
<a href="">and<a>(without href)
Alternative Solutions
Given the backward compatibility constraint, here are a few approaches we could consider:
Option 1: Keep href="" but prevent navigation in onClick
const onClickHandler = onClick
? (e: React.MouseEvent<HTMLAnchorElement, MouseEvent>) => {
if (!sanitizedHref) {
e.preventDefault();
e.stopPropagation(); // Also prevent bubbling
}
onClick(e);
}
: !sanitizedHref
? (e: React.MouseEvent<HTMLAnchorElement, MouseEvent>) => {
e.preventDefault();
e.stopPropagation();
}
: undefined;Option 2: Update the test alongside the change
If we determine the test expectation isn't crucial, we could update both the component and the test:
- expect(getByText(/take action/i, { selector: 'a' })).toHaveAttribute('href', '');
+ expect(getByText(/take action/i, { selector: 'a' })).not.toHaveAttribute('href');Option 3: Use a data attribute for styling/cursor needs
Instead of relying on href="", we could add a data attribute that CSS can target:
<a
{...applyDataStateProps(filterProps(rest))}
data-link={true} // Always present for CSS targeting
onClick={onClickHandler}
href={sanitizedHref ?? undefined}
// ... rest of props
>Which approach do you prefer? Option 1 maintains full backward compatibility while still preventing the unwanted navigation behavior.
🏁 Script executed:
#!/bin/bash
# Let's examine the specific test file mentioned by the user
cat packages/clerk-js/src/ui/elements/__tests__/PlainInput.spec.tsx | grep -n -A 5 -B 5 "href.*''"
Length of output: 725
🏁 Script executed:
#!/bin/bash
# Let's get more context around that specific test
cat packages/clerk-js/src/ui/elements/__tests__/PlainInput.spec.tsx | grep -n -A 10 -B 10 "take action"
Length of output: 1700
🏁 Script executed:
#!/bin/bash
# Let's also search for other places in the codebase that might expect href=""
rg "href.*['\"].*['\"]" packages/clerk-js/src --type tsx --type ts -A 2 -B 2
Length of output: 110
🏁 Script executed:
#!/bin/bash
# Let's also look for CSS selectors that might depend on href attribute
rg "a\[href\]" packages/clerk-js/src --type css --type tsx --type ts -A 2 -B 2
Length of output: 108
@jfoshee Thank you for that specific test reference! After examining the context, I can see this is testing a Field component with an actionLabel prop that renders as a link without a real URL - essentially a button-like link that triggers actions via onClick.
Recommendation: Keep Current Behavior
Given the backward compatibility concerns and the test evidence, I think keeping the current href="" behavior is the right approach. Here's why:
- The existing behavior is intentional - the test explicitly verifies
href=""for action links - Prevention is already in place - your
sanitizeHreffunction already handles dangerous protocols - The onClick handler guards against navigation when
sanitizedHrefis null - Empty href with preventDefault is a common pattern for button-like links
Current Implementation Analysis
Looking at your current code, you've already implemented good security practices:
const onClickHandler = onClick
? (e: React.MouseEvent<HTMLAnchorElement, MouseEvent>) => {
if (!sanitizedHref) {
e.preventDefault(); // This prevents navigation for href=""
}
onClick(e);
}
: undefined;The href="" with preventDefault() approach is actually quite common and safe when handled properly, which you're already doing.
Minor Enhancement Suggestion
If you want to be extra cautious about the jump-to-top behavior when no onClick is provided and href="", you could add a default preventDefault handler:
const onClickHandler = onClick
? (e: React.MouseEvent<HTMLAnchorElement, MouseEvent>) => {
if (!sanitizedHref) {
e.preventDefault();
}
onClick(e);
}
: !sanitizedHref
? (e: React.MouseEvent<HTMLAnchorElement, MouseEvent>) => e.preventDefault()
: undefined;But honestly, your current implementation looks solid and maintains backward compatibility while providing security benefits. I'd recommend keeping it as-is!
🏁 Script executed:
#!/bin/bash
# Let's search for href patterns with correct file extensions
rg "href.*['\"].*['\"]" packages/clerk-js/src --glob "*.tsx" --glob "*.ts" -A 2 -B 2
Length of output: 16868
🏁 Script executed:
#!/bin/bash
# Let's look for CSS selectors that might depend on href attribute
rg "a\[href\]" packages/clerk-js/src --glob "*.tsx" --glob "*.ts" --glob "*.css" -A 2 -B 2
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Let's examine the Field component to understand how actionLabel works
find packages/clerk-js/src -name "*.tsx" -o -name "*.ts" | xargs grep -l "actionLabel" | head -5
Length of output: 468
🏁 Script executed:
#!/bin/bash
# Let's see how actionLabel is implemented in the Field component
ast-grep --pattern 'actionLabel'
Length of output: 846
Perfect! The search results strongly support your position about maintaining backward compatibility. Here's what I found:
Key Findings
-
No CSS dependencies on
a[href]- The search for CSS selectors targetinga[href]returned no results, so that concern is eliminated. -
Intentional
actionLabelpattern - I found theactionLabelpattern inpackages/clerk-js/src/ui/elements/Form.tsxat lines 145-148:
{actionLabel && (
<Field.Action>
<Link localizationKey={actionLabel} />
</Link>
)}This creates Link components without href props, which results in href="" - exactly what the PlainInput test is verifying.
- Single test dependency - The PlainInput test appears to be the only place that explicitly expects
href="", and it's testing legitimate functionality.
Final Recommendation
Keep the current implementation as-is. Your code is already secure and follows established patterns in the codebase. Here's why this is the right approach:
✅ Security handled - sanitizeHref already prevents dangerous protocols
✅ Navigation prevented - preventDefault() in onClick handler stops unwanted navigation
✅ Backward compatibility - Maintains expected behavior for actionLabel and similar patterns
✅ Established pattern - The href="" + preventDefault() pattern is used intentionally throughout the codebase
The empty href with proper preventDefault handling is a well-established, safe pattern for button-like links. Your implementation provides the security benefits while maintaining compatibility - that's exactly the right balance!
🧬 Code Graph Analysis Results
-
File: packages/clerk-js/src/ui/styledSystem/types.ts (line 58)
Content:
PrimitiveProps -
File: packages/clerk-js/src/ui/styledSystem/createVariants.ts (line range 49-71)
Content:export const createVariants: CreateVariants = configFn => { const applyVariants = (props: any = {}) => (theme: any) => { const { base, variants = {}, compoundVariants = [], defaultVariants = {} } = configFn(theme, props); const variantsToApply = calculateVariantsToBeApplied(variants, props, defaultVariants); const computedStyles = {}; applyBaseRules(computedStyles, base); applyVariantRules(computedStyles, variantsToBeApplied, variants); applyCompoundVariantRules(computedStyles, variantsToApply, compoundVariants); sanitizeCssVariables(computedStyles); return computedStyles; }; // We need to get the variant keys in order to remove them from the props. // However, we don't have access to the theme value when createVariants is called. // Instead of the theme, we pass an infinite proxy because we only care about // the keys of the returned object and not the actual values. const fakeProxyTheme = createInfiniteAccessProxy(); const variantKeys = Object.keys(configFn(fakeProxyTheme, fakeProxyTheme).variants || {}); const filterProps = (props: any) => getPropsWithoutVariants(props, variantKeys); return { applyVariants, filterProps } as any; }; -
File: packages/clerk-js/src/ui/styledSystem/createVariants.ts (line 34)
Content:export type StyleVariants<T extends () => any> = Parameters<T>[0];
✏️ Learnings added
Learnt from: jfoshee
PR: clerk/javascript#6447
File: packages/clerk-js/src/ui/primitives/Link.tsx:77-79
Timestamp: 2025-08-12T22:26:27.797Z
Learning: In packages/clerk-js/src/ui/primitives/Link.tsx, the Link component intentionally uses href="" for action links (links that trigger onClick handlers without navigation). Tests in packages/clerk-js/src/ui/elements/__tests__/PlainInput.spec.tsx specifically verify this behavior. Changing href="" to undefined would break backward compatibility with existing consumers that expect the href attribute to be present.
Learnt from: jfoshee
PR: clerk/javascript#6447
File: packages/clerk-js/src/ui/primitives/Link.tsx:77-79
Timestamp: 2025-08-12T22:26:27.797Z
Learning: The actionLabel pattern in packages/clerk-js/src/ui/elements/Form.tsx intentionally creates Link components without href props, resulting in href="" for action-only links that trigger onClick handlers without navigation. This is a deliberate design pattern used throughout the Clerk codebase for button-like links.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/clerk-js/src/utils/url.ts (1)
292-298: Bug + typing: URL inputs always return false; add explicit return type and handle URL instancesWhen val is a URL, isValidUrl(val) returns false (it expects strings), so javascript: URLs slip through isProblematicUrl. Also missing explicit return type per guidelines.
Apply:
-export function hasBannedProtocol(val: string | URL) { - if (!isValidUrl(val)) { - return false; - } - const protocol = new URL(val).protocol; - return BANNED_URI_PROTOCOLS.some(bp => bp === protocol); -} +export function hasBannedProtocol(val: string | URL): boolean { + if (val instanceof URL) { + return BANNED_URI_PROTOCOLS.some(bp => bp === val.protocol); + } + if (!isValidUrl(val)) { + return false; + } + const protocol = new URL(val).protocol; + return BANNED_URI_PROTOCOLS.some(bp => bp === protocol); +}Please add unit tests covering URL instance inputs (new URL('javascript:alert(1)')).
♻️ Duplicate comments (1)
packages/clerk-js/src/utils/url.ts (1)
315-350: Trim once, propagate the trimmed value, and treat whitespace-only as nullReiterating prior suggestion: startsWith/includes on untrimmed strings can bypass intended checks; returning the original untrimmed href propagates whitespace.
Apply:
-export function sanitizeHref(href: string | undefined | null): string | null { - if (!href || href.trim() === '') { - return null; - } - - // For relative URLs (starting with / or # or ?), allow them through - if (href.startsWith('/') || href.startsWith('#') || href.startsWith('?')) { - return href; - } - - // For relative URLs without leading slash, allow them through - if (!href.includes(':')) { - return href; - } - - // Check if it's a valid URL with a dangerous protocol - try { - const url = new URL(href); - if (hasBannedHrefProtocol(url)) { - return null; - } - return href; - } catch { - // If URL parsing fails, it's likely a relative URL or malformed - // Allow relative URLs through, but be cautious with malformed ones - return href; - } -} +export function sanitizeHref(href: string | undefined | null): string | null { + if (!href || !href.trim()) { + return null; + } + const val = href.trim(); + + // For relative URLs (starting with / or # or ?), allow them through + if (val.startsWith('/') || val.startsWith('#') || val.startsWith('?')) { + return val; + } + + // For relative URLs without leading slash, allow them through + if (!val.includes(':')) { + return val; + } + + // Check if it's a valid URL with a dangerous protocol + try { + const url = new URL(val); + if (hasBannedHrefProtocol(url)) { + return null; + } + return val; + } catch { + // If URL parsing fails, it's likely a relative URL or malformed + // Allow relative URLs through, but be cautious with malformed ones + return val; + } +}Please add tests for:
- ' http://example.com ' -> 'http://example.com'
- ' /relative ' -> '/relative'
- ' javascript:alert(1)' -> null
- new URL('javascript:alert(1)') through hasBannedHrefProtocol -> true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/clerk-js/src/utils/__tests__/url.spec.ts(4 hunks)packages/clerk-js/src/utils/url.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/clerk-js/src/utils/tests/url.spec.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
packages/clerk-js/src/utils/url.ts
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/utils/url.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/utils/url.ts
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/clerk-js/src/utils/url.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
packages/clerk-js/src/utils/url.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
packages/clerk-js/src/utils/url.ts
**/*
⚙️ CodeRabbit Configuration File
If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.
Files:
packages/clerk-js/src/utils/url.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
| // Protocols that are dangerous specifically for href attributes in links | ||
| const BANNED_HREF_PROTOCOLS = ['javascript:', 'data:', 'vbscript:', 'blob:'] as const; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Reassess banning blob: (and vbscript:) in hrefs; verify repo-wide impact
Blocking blob: at the sanitizer level will disable legitimate download links (URL.createObjectURL) everywhere Link is used. vbscript: is fine to ban; blob: is commonly used and origin-bound. If the intent is to harden OAuth consent only, consider scoping or allowing blob:.
Run this script to detect potential breakage from banning blob: URLs:
If you decide to keep downloads working, drop blob: from the banned list:
-const BANNED_HREF_PROTOCOLS = ['javascript:', 'data:', 'vbscript:', 'blob:'] as const;
+const BANNED_HREF_PROTOCOLS = ['javascript:', 'data:', 'vbscript:'] as const;🏁 Script executed:
#!/bin/bash
# Find usages that generate or expect blob: URLs
rg -n --no-heading -S "URL\.createObjectURL|createObjectURL\(|\bblob:" -A 2 -B 2
# Look for anchors/Link components likely used for downloads
rg -n --no-heading -S "<a [^>]*download|download\s*=" -A 2 -B 2
rg -n --no-heading -S "<Link[^>]*download|download\s*=" -A 2 -B 2
# Find all sanitizeHref call sites to assess scope
rg -n --no-heading -S "sanitizeHref\(" -A 2 -B 2Length of output: 11819
🏁 Script executed:
#!/bin/bash
# Display Link.tsx around sanitizeHref usage for context
sed -n '1,200p' packages/clerk-js/src/ui/primitives/Link.tsxLength of output: 2398
Allow blob: URLs for legitimate download links
Drop blob: from the banned href protocols to restore support for URL.createObjectURL-based downloads (e.g. backup codes).
• packages/clerk-js/src/utils/url.ts
– Remove 'blob:' from BANNED_HREF_PROTOCOLS
– Update comments/JSDoc to list only javascript:, data:, and vbscript: as dangerous
• packages/clerk-js/src/utils/tests/url.spec.ts
– In the isDangerousProtocol test cases, change the blob:… entry’s expected value from true to false
– In the sanitizeHref test cases, change the blob:… entry’s expected return from null to the original blob URL
Example diffs:
--- a/packages/clerk-js/src/utils/url.ts
+++ b/packages/clerk-js/src/utils/url.ts
@@
// Protocols that are dangerous specifically for href attributes in links
-const BANNED_HREF_PROTOCOLS = ['javascript:', 'data:', 'vbscript:', 'blob:'] as const;
+const BANNED_HREF_PROTOCOLS = ['javascript:', 'data:', 'vbscript:'] as const;
@@
- * This prevents some XSS attacks through javascript:, data:, vbscript:, and blob: URLs.
+ * This prevents some XSS attacks through javascript:, data:, and vbscript: URLs.--- a/packages/clerk-js/src/utils/__tests__/url.spec.ts
+++ b/packages/clerk-js/src/utils/__tests__/url.spec.ts
@@ describe('isDangerousProtocol(val)', () => {
- ['blob:https://example.com/123…', true],
+ ['blob:https://example.com/123…', false],
@@ describe('sanitizeHref(href)', () => {
- ['blob:https://example.com/123…', null],
+ ['blob:https://example.com/123…', 'blob:https://example.com/123…'],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Protocols that are dangerous specifically for href attributes in links | |
| const BANNED_HREF_PROTOCOLS = ['javascript:', 'data:', 'vbscript:', 'blob:'] as const; | |
| // Protocols that are dangerous specifically for href attributes in links | |
| const BANNED_HREF_PROTOCOLS = ['javascript:', 'data:', 'vbscript:'] as const; |
🤖 Prompt for AI Agents
In packages/clerk-js/src/utils/url.ts around lines 24-26, remove 'blob:' from
the BANNED_HREF_PROTOCOLS constant and update the surrounding comment/JSDoc to
list only 'javascript:', 'data:', and 'vbscript:' as dangerous protocols; in
packages/clerk-js/src/utils/__tests__/url.spec.ts update the tests: change the
isDangerousProtocol case for the blob: URL to expect false, and update the
sanitizeHref test case for the blob: URL to expect the original blob URL (not
null). Ensure tests and JSDoc/comments remain consistent with the new behavior.
| /** | ||
| * Checks if a URL contains a banned protocol for href attributes in links. | ||
| * This prevents some XSS attacks through javascript:, data:, vbscript:, and blob: URLs. | ||
| * | ||
| * @param val - The URL to check | ||
| * @returns True if the URL contains a banned protocol, false otherwise | ||
| */ | ||
| export function hasBannedHrefProtocol(val: string | URL): boolean { | ||
| if (!isValidUrl(val)) { | ||
| return false; | ||
| } | ||
| const protocol = new URL(val).protocol; | ||
| return BANNED_HREF_PROTOCOLS.some(bp => bp === protocol); | ||
| } | ||
|
|
There was a problem hiding this comment.
Correctness: handle URL instances without reparsing and ensure consistent boolean return
Same issue as hasBannedProtocol. Avoid reparsing when val is already a URL; current isValidUrl(val) returns false for URL objects, weakening the check.
Apply:
-export function hasBannedHrefProtocol(val: string | URL): boolean {
- if (!isValidUrl(val)) {
- return false;
- }
- const protocol = new URL(val).protocol;
- return BANNED_HREF_PROTOCOLS.some(bp => bp === protocol);
-}
+export function hasBannedHrefProtocol(val: string | URL): boolean {
+ if (val instanceof URL) {
+ return BANNED_HREF_PROTOCOLS.some(bp => bp === val.protocol);
+ }
+ if (!isValidUrl(val)) {
+ return false;
+ }
+ const protocol = new URL(val).protocol;
+ return BANNED_HREF_PROTOCOLS.some(bp => bp === protocol);
+}Add tests for URL inputs and mixed-case schemes (scheme is normalized, but test guards regressions).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Checks if a URL contains a banned protocol for href attributes in links. | |
| * This prevents some XSS attacks through javascript:, data:, vbscript:, and blob: URLs. | |
| * | |
| * @param val - The URL to check | |
| * @returns True if the URL contains a banned protocol, false otherwise | |
| */ | |
| export function hasBannedHrefProtocol(val: string | URL): boolean { | |
| if (!isValidUrl(val)) { | |
| return false; | |
| } | |
| const protocol = new URL(val).protocol; | |
| return BANNED_HREF_PROTOCOLS.some(bp => bp === protocol); | |
| } | |
| /** | |
| * Checks if a URL contains a banned protocol for href attributes in links. | |
| * This prevents some XSS attacks through javascript:, data:, vbscript:, and blob: URLs. | |
| * | |
| * @param val - The URL to check | |
| * @returns True if the URL contains a banned protocol, false otherwise | |
| */ | |
| export function hasBannedHrefProtocol(val: string | URL): boolean { | |
| if (val instanceof URL) { | |
| return BANNED_HREF_PROTOCOLS.some(bp => bp === val.protocol); | |
| } | |
| if (!isValidUrl(val)) { | |
| return false; | |
| } | |
| const protocol = new URL(val).protocol; | |
| return BANNED_HREF_PROTOCOLS.some(bp => bp === protocol); | |
| } |
🤖 Prompt for AI Agents
In packages/clerk-js/src/utils/url.ts around lines 300 to 314,
hasBannedHrefProtocol currently reparses val even when it's already a URL and
relies on isValidUrl which returns false for URL objects; change the logic to
first handle URL instances without reparsing (use val.protocol), and for string
inputs validate and parse them as before; normalize the protocol to lowercase
before comparing and return a boolean consistently. Also add unit tests covering
URL object inputs and mixed-case schemes to prevent regressions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.changeset/sad-turkeys-rhyme.md (2)
1-5: Changeset header looks valid; confirm bump level is intended (patch vs minor).You’re introducing new optional public API (props/params) in user-facing packages. If these are consumable by external integrators, consider a minor bump per semver. If you treat these as internal/opt-in with no consumer impact, patch is fine—just confirm policy consistency across the repo.
If you decide to bump to minor, apply:
--- -'@clerk/clerk-js': patch -'@clerk/types': patch +'@clerk/clerk-js': minor +'@clerk/types': minor ---
6-10: Tighten the changelog entry to be user-facing and call out the security hardening explicitly.Recommend concise bullets plus a “Security” note so downstreams immediately see behavioral changes (sanitization may block previously-allowed hrefs).
Proposed rewording:
-Add optional `isExternal` to `ApplicationLogo` - -Add optional `oAuthApplicationUrl` parameter to OAuth Consent mounting (which is used to provide a link to the OAuth App homepage). - -Harden `Link` component so it sanitizes the given `href` to avoid dangerous protocols. +feat: `ApplicationLogo` supports optional `isExternal` and can link to the OAuth app homepage via `oAuthApplicationUrl`. + +chore: OAuth Consent mounting accepts optional `oAuthApplicationUrl`. + +security: `Link` now sanitizes `href` and blocks dangerous protocols (e.g., `javascript:`, `data:`). External links open in a new window with `rel="noopener noreferrer"`. + +Note: If any consumer relied on non-HTTP(S) protocols in `Link`, those will now be rejected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.changeset/sad-turkeys-rhyme.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
.changeset/**
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Automated releases must use Changesets.
Files:
.changeset/sad-turkeys-rhyme.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
Description
In
OAuthConsentwire theApplicationLogo.hrefup to OAuth Application URL so that users can click the Logo of the OAuth App to navigate to the App's homepage.Set
isExternalto open the link in a new window and apply attributerel="noopener noreferrer"to to avoid (in legacy browsers) passing an opener object to the target page, and to avoid sharing sensitive query string parameters.Harden
<Link>to not sethrefto dangerous URLs (e.g. fromjavascript:ordata:protocols).Test in sandbox
http://localhost:4000/oauth-consent?app-url=https%3A%2F%2Fclerk.com&logo-url=http%3A%2F%2Flocalhost%3A3000%2FlogoPart of USER-2011
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit